fix: switch to gen-lsp-types#2494
Conversation
[gen-lsp-types](https://github.com/ribru17/gen-lsp-types) is an alternative to the lsp-types crate, with types generated via codegen from the official LSP Metamodel for correctness and completeness. lsp-types issues fixed in gen-lsp-types: - gluon-lang/lsp-types#310 - gluon-lang/lsp-types#308 - gluon-lang/lsp-types#284 - gluon-lang/lsp-types#278 - gluon-lang/lsp-types#277 - gluon-lang/lsp-types#260 - gluon-lang/lsp-types#245 - gluon-lang/lsp-types#93 Closes Myriad-Dreamin#1542 (as an alternative solution)
|
See rust-lang/rust-analyzer#22115 and wgsl-analyzer/wgsl-analyzer#1090 for other migration considerations |
Myriad-Dreamin
left a comment
Myriad-Dreamin
left a comment
There was a problem hiding this comment.
Hi, thanks for the awesome gen-lsp-types crate. I really appreciate you providing a replacement for the unmaintained lsp-types. I've left two comments, but they shouldn't block merging.
| let handle = |s, method: &str, params: JsonValue| { | ||
| let Some(handler) = self.notifications.get(method) else { | ||
| let Some(handler) = self.notifications.get(&method.to_owned().into()) else { | ||
| log::warn!("unhandled notification: {method}"); |
There was a problem hiding this comment.
Nit: every log of request/notification method introduces an extra allocation. while it doesn't matter much, but I feel it is kind of imperfect:
impl fmt::Display for LspNotificationMethod {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s: String = self.clone().into(); // a clone
write!(f, "{s}")
}
}There was a problem hiding this comment.
Good point. I think the best thing to do is to move from a custom variant of Cow<'static, str> to just &str, where the reference lifetime is a parameterized 'a. That way, the enum becomes Copy and can be constructed from any &str without any allocations, while still allowing exhaustive enum-style pattern matching. What do you think?
| let is_disconnect = method == dapts::request::Disconnect::COMMAND; | ||
|
|
||
| let Some(handler) = self.requests.get(method) else { | ||
| let Some(handler) = self.requests.get(&method.to_owned().into()) else { |
There was a problem hiding this comment.
I'm not pretty in flavor of converting string enums into LspRequestMethod or LspNotificationMethod everywhere. And this case is even worse, for a DAP server, every "DapCommandMethod" and "DapEventMethod" are converted to LspRequestMethod::Custom and LspNotificationMethod::Custom and requires extra allocations.
Context: LSP and DAP both share a same base protocol.
What's your idea?
There was a problem hiding this comment.
Good point... maybe it's better to remove the typing guarantee and keep these keys as &str. This pain point will be largely mitigated by implementing the suggestion from my previous comment, but the Custom dap request/notification variants will still be present since the LSP metamodel is unaware of the DAP specification.
2 participants
gen-lsp-types is an alternative to the lsp-types crate, with types generated via codegen from the official LSP Metamodel for correctness and completeness.
lsp-types issues fixed in gen-lsp-types:
SnippetTextEditsupport (proposed) gluon-lang/lsp-types#310Uritype in 0.96.0 is hard to use and have almost no interoperability gluon-lang/lsp-types#284DefaultforCodeActionOptionsgluon-lang/lsp-types#260Closes #1542 (as an alternative solution)